Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Allow disabling native SDK initialization but still use it #1259

Merged
merged 3 commits into from
Jan 4, 2021

Conversation

jennmueng
Copy link
Member

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Allows the user to disable the initialization of the Native SDK in the case that they already have one initialized themselves. We do this by adding the shouldInitializeNativeSdk key to ReactNativeOptions. This is not breaking as it will default to true.

💡 Motivation and Context

User has an app that is extensively native iOS and the React Native section is only run way later: #1248

💚 How did you test it?

Wrote a new test in wrapper.ts.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

🔮 Next steps

Should add documentation on this option.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 22, 2020

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against dd56271

@jennmueng jennmueng self-assigned this Dec 22, 2020
@jennmueng jennmueng changed the title feat: Allow disabling native SDK initialization but still use native feat: Allow disabling native SDK initialization but still use it Dec 22, 2020
@jennmueng jennmueng requested a review from a team December 22, 2020 06:11
Comment on lines 23 to 27
/** Initializes the native SDK on init.
* Set this to `false` if you have an existing native SDK and don't want to re-initialize.
* @default true
*/
shouldInitializeNativeSdk?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are the edge cases of such flag?

eg 1. RN SDK captures an event, Native SDKs are not inited.
2. RN sets up X DSN but events go to Y DSN etc...

not against it, but we'll need to figure out such edge cases and write them down.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree, should mention any possible side effects and edge cases, and also need to document this clearly. Thanks for the catch!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being the author of the initial feature request, I'd like to add my 2 cents: Having the only source for DSN/environment etc would be a better option, so that I don't need to worry about that in the JS side, since it'd've been already fully initialised and configured on the native side.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pioneer32 that would be the perfect solution, agree, but I'd say we'd go the other way around.
we'd sync the options from RN -> Native as we already do, which is let's say ~90+% of the use cases.
you could open an issue as a feature request and let's see if it gets upvoted, but I'd bet on the case described above.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marandaneto Thank you for clarification! My guess is shouldInitializeNativeSdk sort of going to disable calling

[SentrySDK startWithOptionsObject:sentryOptions];
somehow (I might be wrong though). However, being keen to see what it'd look like, I commented out that line intentionally, and it seems to work as expected, almost :) FYI The problem I encountered was: all the tags/breadcrumbs/etc set to the native client are NOT copied into JS client before sending an error from JS.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bruno-garcia
Copy link
Member

For reference, this came up in Flutter too already: getsentry/sentry-dart#255 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants